-
Notifications
You must be signed in to change notification settings - Fork 83
Support running DWDS without a Chrome Debug Port (web-socket-based) #2639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow-up (in a different change) should we update the ChromeProxyService to also use this Built object for service extensions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ChromeProxyService leverages the inspector to evaluate JavaScript expressions via invokeExtensionJsExpression
(as seen in the _callServiceExtension
method). This pattern is consistently used throughout ChromeProxyService for all interactions with the dartDevEmbedder. It evaluates JavaScript expressions directly in the browser context rather than sending structured messages. Since ChromeProxyService operates by executing JavaScript in the browser, using a Built object here wouldn't align with ChromeProxyService's JavaScript evaluation approach. The current pattern maintains consistency with how ChromeProxyService handles all other browser interactions.
remoteDebugger.onClose.first.then((_) => close()); | ||
} | ||
} catch (_) { | ||
// Chrome proxy service not available in WebSocket-only mode - ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exceptions are we now catching and ignoring here?
If we're in "websocket-only mode" (does this mean -d web-server
?) then I would expect the debugger to never even be connected. But if we're not in websocket-only mode (-d chrome
?) then it seems like it should at least be a warning if we get an exception here.
Can you add more context on what this is doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was because we were throwing an UnsupportedError in the web_socket_app_debug_services when accessing chromeProxyService. I have changed the logic to return null instead, so we can now simply do: _appDebugServices.chromeProxyService?.remoteDebugger.onClose.first.then((_) { close(); });
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there could be a debugger connected when in "web-server" mode after the application starts if the user manually connects it using the Dart DevTools chrome extension.
Actually now I think I agree, what errors are we swallowing here?
@@ -22,9 +22,20 @@ class DebugConnection { | |||
Future<void>? _closed; | |||
|
|||
DebugConnection(this._appDebugServices) { | |||
_appDebugServices.chromeProxyService.remoteDebugger.onClose.first.then((_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this all just be:
_appDebugServices.chromeProxyService?.remoteDebugger.onClose.first.then((_) { close(); });
All the new code here looks to be doing all the same checks as close
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! I've simplified the constructor now
} on StateError catch (e) { | ||
// The sink has already closed (app is disconnected), or another StateError occurred. | ||
_logger.warning( | ||
'Failed to send request to client, connection likely closed. Error: $e', | ||
'Failed to send request to client ${injectedConnection.hashCode}, ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the hashCode
actually add any information to these messages? I'm assuming the injected clients don't override hashCode so it'll just be a random number every time.
debugService.chromeProxyService as ChromeProxyService; | ||
/// Common interface for debug service containers. | ||
abstract class IAppDebugServices { | ||
dynamic get debugService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to my suggestion with the proxy services, these should all have common interfaces that we can use here rather than dynamic
.
I suspect that with those interfaces AppDebugServices
doesn't even need to be abstract. We should just be able to keep the concrete one as it was before. I'm imagining the only change is changing ChromeProxyService chromeProxyService
to ProxyService proxyService
.
typedef SendClientRequest = int Function(Object request); | ||
|
||
// Connection control for WebSocket clients | ||
bool _acceptNewConnections = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is always true
, I don't see it ever getting updated to false. So I don't think it's doing anything. Can we delete this and the code using it below?
_clientsConnected
seems to be in the same boat.
} | ||
|
||
// Creates a random auth token for more secure connections. | ||
String _makeAuthToken() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's moved this to a shared location and use it both here and in debug_service.dart
. Maybe we need a new service_utils.dart
file here.
} | ||
|
||
/// WebSocket-based VM service proxy for web debugging. | ||
class WebSocketProxyService implements VmServiceInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the duplicated code here from ChromeProxyService
can go on the abstract ProxyService
and then both of these can just inherit those members.
}; | ||
} | ||
|
||
static Map<String, Object> _extDwdsEmitEventHandler(VmResponse request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move all these to a shared file. Or probably even better just move WebSocketDwdsVmClient
into dwds_vm_client.dart
. Then it can share those private members.
await chromeProxyService.remoteDebugger.close(); | ||
} | ||
} catch (_) { | ||
// Chrome proxy service not available in WebSocket-only mode - ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like in the setup, it feels bad to see errors being swallowed like this. Can we catch the error we know we want to ignore and surface any others?
Future<Map<String, dynamic>?> handleServiceExtension( | ||
String method, | ||
Map<String, dynamic> args, | ||
) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a missing await somewhere in this method?
await webSocketProxyService.isInitialized; | ||
_logger.fine('WebSocket proxy service initialized successfully'); | ||
} else { | ||
_logger.warning('WebSocket proxy service is null'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there ever an instance where this is okay? Should we throw/assert instead? Similar question below.
appConnection.runMain(); | ||
_mainHasStarted = true; | ||
} catch (e) { | ||
if (e.toString().contains('Main has already started')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to catch this? I would assume the check on line 900 prevents this from occurring.
Map<String, dynamic> args, | ||
) async { | ||
if (method == 'ext.flutter.reassemble') { | ||
await _dartDevEmbedder.debugger.maybeInvokeFlutterReassemble(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is handled by the calling application i.e. Flutter tools and we shouldn't need to call this ourselves in DWDS. This is how the Chrome-based implementation works. Is there a reason to change that?
disassemble
via hot restart should be the same but isn't currently so that makes sense to leave alone.
WebSocketProxyService
class that implements VM service protocol over WebSockets.WebSocketProxyService
,WebSocketDebugService
,WebSocketAppDebugServices
, andWebSocketDwdsVmClient
to support socket-based DWDS functionality.DevHandler
withuseWebSocketConnection
flag to toggle between Chrome-based and WebSocket-based communication protocols.Required Flutter Tools changes: flutter/flutter#171648